-
-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix race conditions due to zero-copy semantics of pipe transport. #261
Conversation
c133ed2
to
21f2649
Compare
Tests are timing out, but passing on my machine. @zenhack Can you confirm you're getting the same thing? It's timing out in Thoughts on how to proceed? |
I have only skimmed this, but the design seems a bit fragile to me, at least in terms of preserving the desired semantics -- we really want the buffer size to specify a number of messages, not a number of bytes. What if instead of returning a pair of rwcs, we returned a pair of Codecs, but instead of just sending the messages themselves down a channel, we call Marshal/Unmarshal before sending/receiving? |
I'm not sure I follow. Where might this break, in practice? The semantics of
That ought to work, but it's not clear to me what we gain from the added complexity. I understand that it gives us (bounded?) buffering of messages, but what problem is that solving? |
P.S.: I'm also of a mind that if buffering causes higher-level code to misbehave, the bug in the higher level code, and the buffered pipe is a valuable canary. As a matter of design, Of course, it's possible that I'm overlooking something, so I'm interested to hear your thoughts 🙂 |
Ah, that makes sense. Yeah, if nothing is depending on not buffering then that's different. That said, the lack of buffering in net.Pipe() has caught a couple deadlocks, so I wonder if we should keep it unbuffered where we can? In that case I won't die on this hill, but I think the thing I described is actually simpler (especially as a delta from the current implementation): it's just two channels and a call to .Marshal()/Marshal() on either side |
There was exactly one test that was using a synchronous pipe. I've replaced it with
Ah, I had something more complicated in mind. I'll give it a whirl. |
21f2649
to
0b23f52
Compare
NewPipe now returns a Codec.
0b23f52
to
ce8a15c
Compare
While investigating the race-condition triggered by #260, it was determined that the zero-copy semantics of
transport.NewPipe
were causing data races. In the observed case, both sides of the transport connection were concurrently callingMessage.Release()
. In a hitherto unobserved case, it is also possible for data races to emerge if one side of the transport connection mutates a message segment.This PR addresses the above design flaw by removing
transport.NewPipe
and creating a new internal package calledbufpipe
. Rather than implement theTransport
interface,bufpipe.New
returns a pair ofio.ReadWriteCloser
s, which can be consumed byrpc.NewStreamTransport
. Similarly, tests requiring unbuffered transport may use the standard-librarynet.Pipe
.An important conclusion of this investigation is that an in-process, zero-copy transport is not feasible under the current design.